-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set priority for multiple files in one WebAPI request #9541
Conversation
src/webui/api/torrentscontroller.cpp
Outdated
torrent->setFilePriority(fileID, priority); | ||
BitTorrent::TorrentHandle *const torrent = BitTorrent::Session::instance()->findTorrent(hash); | ||
if (torrent && torrent->hasMetadata()) { | ||
for (const auto &fileID : fileIDs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really, QString isn't much longer to type than auto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any benefit to using an explicit type vs auto
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe CODING_GUIDELINES has enough explaination: https://github.com/qbittorrent/qBittorrent/blob/master/CODING_GUIDELINES.md#9-misc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendations for modern C++ are to use auto
quite liberally. We get no benefit from an explicit type here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendations for modern C++ are to use auto quite liberally.
Source?
IMO you're using auto
a bit too liberal. auto
is more like a shortcut, not be used like var
in javascript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From Item 5: Prefer auto to explicit type declarations in Effective Modern C++ by Scott Meyers:
Using [auto] saves typing, sure, but it also prevents correctness and performance issues that can bedevil manual type declaration ... There are thus several reasons to prefer auto over explicit type declarations ... The fact of the matter is that writing types explicitly often does little more than introduce opportunities for subtle errors, either in correctness or efficiency or both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you, however I don't see he mentions anything about readability, a google search gives a handful of debates about readability of using auto
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we discussed our rules about using auto
keyword, we wanted to find a balance between faster code writing and readability. So don't abuse it, please.
src/webui/api/torrentscontroller.cpp
Outdated
int fileID = params()["id"].toInt(); | ||
int priority = params()["priority"].toInt(); | ||
BitTorrent::TorrentHandle *const torrent = BitTorrent::Session::instance()->findTorrent(hash); | ||
const QStringList fileIDs {params()["id"].split('|')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change param name, but it breaks API compatibility...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want your thoughts: we add an optional ids
param that takes priority over the id
param. Or we can change the name and bump the web api version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't produce incompatible changes so frequently if possible.
So maybe we should have some way to keep track of these "non-urgent" changes, and then make all the changes at once when we have the required changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe keep the list of postponed changes near the API version declaration, e.g. "When your changes require increasing of minor version you should also apply the following:" and then list of changes.
What do you say? @Chocobo1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you say? @Chocobo1?
I don't think I get your idea.
However I think the commit bumping API version should also include the api code changes, so it is possible to look up by reading git log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I get your idea.
@Chocobo1, take this case for example.
Meaning of param 'id' was changed. Now it provide list of file IDs. It should cause changing of param name but it breaks API compatibility. It's really bad idea to make incompatible changes too frequently.
We can proceed to use old name for some time and just mark it for future changing.
When we make changes that cannot be made in a compatible way, we can add all pending changes with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can proceed to use old name for some time and just mark it for future changing.
I don't object the idea, however I must say from my experience, revisiting an older code after some time takes much more time for the developer and outcome is usually not good enough quality.
If @Piccirello is interested, we could try it out in this case (experiment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
however I must say from my experience, revisiting an older code after some time takes much more time for the developer and outcome is usually not good enough quality.
So I suggest to keep list with postponed changes. We can have some instructions, where, what and how it should be changed. Or we can even have commented changes inplace.
Well, I'm not insisting. I just thought we shouldn't break API too often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be interested, maybe maintaining a separate branch with breaking api changes that can be merged pre-minor version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe maintaining a separate branch with breaking api changes that can be merged pre-minor version bump.
And have opened PR for it.
Let's try something!
00f64e6
to
f23dad0
Compare
src/webui/api/torrentscontroller.cpp
Outdated
} | ||
|
||
for (const auto &id : fileIdsInts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const int id : fileIdsInts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the coding guidelines:
Declarations for which one can gather enough information about the object interface (type) from its name or the usage pattern .. or the right part of the expression nicely fit here.
Seems we can gather enough info about the type from the variable name fileIdsInts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name does not matter. This paragraph means something completely different:
auto *varName = static_cast<RSS::Feed *>(rssItemPtr);
Or:
auto *session = BitTorrent::Session::instance();
8292784
to
995d92b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Piccirello, you forgot to fix this.
@glassez whoops, good catch. Pushing a new commit. |
d08519b
to
dba8b59
Compare
e6d3a11
to
942dcbd
Compare
1c80468
to
33942f2
Compare
33942f2
to
9ff6e9c
Compare
9ff6e9c
to
9a1e415
Compare
5e4e2b4
to
76bb830
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chocobo1, feel free to merge it.
Priority select boxes would frequently go blank due to an unexpected priority value. On first load, the torrent-scoped file checkbox's state was inconsistent with the state of the torrent's files.
76bb830
to
57e6254
Compare
Thank you! |
This API was never changed during the API revamp some months back
Closes #6259